Skip to content

SelectLineScreenの分割リファクタリング#5368

Merged
TinyKitten merged 1 commit intodevfrom
chore/select-line-screen-refactoring
Feb 20, 2026
Merged

SelectLineScreenの分割リファクタリング#5368
TinyKitten merged 1 commit intodevfrom
chore/select-line-screen-refactoring

Conversation

@TinyKitten
Copy link
Member

@TinyKitten TinyKitten commented Feb 20, 2026

Summary by CodeRabbit

  • テスト: 駅の初期化、路線選択、プリセットカルーセルデータ取得、駅キャッシング機能、ウォークスルーガイドに関する包括的なテストを追加しました。

  • リファクター: 駅と路線の選択フローを改善し、プリセットカルーセルのデータ管理を最適化し、駅データのキャッシング処理を強化しました。

@TinyKitten TinyKitten self-assigned this Feb 20, 2026
@github-actions github-actions bot added the react label Feb 20, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 20, 2026

📝 Walkthrough

Walkthrough

5つの新規カスタムフック(useInitialNearbyStation、useLineSelection、usePresetCarouselData、useSelectLineWalkthrough、useStationsCache)をsrc/hooks/に追加し、これらをホークスのインデックスにエクスポートしました。SelectLineScreenをリファクタリングして、インラインの状態管理とGraphQLロジックをこれらのフックに移行しました。

Changes

Cohort / File(s) Summary
初期駅取得フック
src/hooks/useInitialNearbyStation.ts, src/hooks/useInitialNearbyStation.test.tsx
背景位置更新を停止し、非同期に最も近い駅を取得するフック。位置座標の取得、駅の取得、初回起動アラートの表示を処理します。
ライン選択フック
src/hooks/useLineSelection.ts, src/hooks/useLineSelection.test.tsx
ライン/路線選択フローを調整するフック。GraphQLクエリ(GET_LINE_STATIONS、GET_LINE_GROUP_STATIONS、GET_STATION_TRAIN_TYPES_LIGHT)を管理し、モーダル表示、駅選択、列車型の選択を処理します。
プリセットカルーセルデータフック
src/hooks/usePresetCarouselData.ts, src/hooks/usePresetCarouselData.test.ts
保存済みルートのカルーセルデータを構築するフック。バルク取得(GET_LINE_LIST_STATIONS_PRESET、GET_LINE_GROUP_LIST_STATIONS_PRESET)でルートの駅情報を取得します。
ラインウォークスルーフック
src/hooks/useSelectLineWalkthrough.ts, src/hooks/useSelectLineWalkthrough.test.tsx
ガイド付きウォークスルーのUI要素ハイライト処理を管理するフック。特定のステップ(changeLocation、selectLine、savedRoutes、customize)でスポットライト領域を設定します。
駅キャッシュフック
src/hooks/useStationsCache.ts, src/hooks/useStationsCache.test.ts
駅の路線データをキャッシュするフック。GET_LINE_LIST_STATIONS_LIGHTでバルク取得し、グローバル状態を更新します。
スクリーンのメインリファクタリング
src/screens/SelectLineScreen.tsx, src/screens/SelectLineScreen.render.test.tsx
SelectLineScreenを新規フックで構成し直しました。インライン状態管理、GraphQLクエリ、AsyncStorageの処理を上記フックに移動。テストスイートを新規に追加して、ローディング状態、モーダル表示、ウォークスルーオーバーレイなど複数のシナリオを検証します。
フックインデックス更新
src/hooks/index.ts
5つの新規フック(useInitialNearbyStation、useLineSelection、usePresetCarouselData、useSelectLineWalkthrough、useStationsCache)をパブリックエクスポートに追加。

Sequence Diagram(s)

sequenceDiagram
    participant SelectLineScreen
    participant useInitialNearbyStation
    participant useFetchNearbyStation
    participant LocationAPI
    participant GraphQL
    participant AsyncStorage
    participant AlertAPI

    SelectLineScreen->>useInitialNearbyStation: マウント
    useInitialNearbyStation->>LocationAPI: hasStartedLocationUpdatesAsync()
    LocationAPI-->>useInitialNearbyStation: true
    useInitialNearbyStation->>LocationAPI: stopLocationUpdatesAsync()
    
    useInitialNearbyStation->>useFetchNearbyStation: 座標で駅を取得
    useFetchNearbyStation->>GraphQL: fetchNearby(座標、limit=1)
    GraphQL-->>useFetchNearbyStation: 駅データ
    useFetchNearbyStation-->>useInitialNearbyStation: 駅
    
    useInitialNearbyStation->>AsyncStorage: getItem('firstAlertShown')
    AsyncStorage-->>useInitialNearbyStation: null
    useInitialNearbyStation->>AlertAPI: Alert.alert('notice')
    AlertAPI-->>useInitialNearbyStation: ユーザー応答
    
    useInitialNearbyStation-->>SelectLineScreen: station, nearbyStationLoading
Loading
sequenceDiagram
    participant SelectLineScreen
    participant useLineSelection
    participant GraphQL
    participant JotaiState
    participant GeoLib

    SelectLineScreen->>useLineSelection: handleLineSelected(line)
    useLineSelection->>GraphQL: GET_LINE_STATIONS(lineId)
    GraphQL-->>useLineSelection: 駅リスト
    useLineSelection->>GeoLib: findNearest(駅, 現在位置)
    GeoLib-->>useLineSelection: 最寄駅
    useLineSelection->>JotaiState: pendingStation, pendingStations, pendingLine更新
    
    SelectLineScreen->>useLineSelection: handleTrainTypeSelect(trainType)
    useLineSelection->>GraphQL: GET_LINE_GROUP_STATIONS(lineGroupId)
    GraphQL-->>useLineSelection: 駅グループデータ
    useLineSelection->>JotaiState: pendingStations, pendingTrainType更新
Loading
sequenceDiagram
    participant SelectLineScreen
    participant usePresetCarouselData
    participant useSavedRoutes
    participant GraphQL
    participant JotaiState

    SelectLineScreen->>usePresetCarouselData: マウント
    usePresetCarouselData->>useSavedRoutes: ルート一覧取得
    useSavedRoutes-->>usePresetCarouselData: SavedRoute[]
    
    usePresetCarouselData->>GraphQL: GET_LINE_LIST_STATIONS_PRESET(lineIds)
    GraphQL-->>usePresetCarouselData: 駅マップ
    
    usePresetCarouselData->>GraphQL: GET_LINE_GROUP_LIST_STATIONS_PRESET(lineGroupIds)
    GraphQL-->>usePresetCarouselData: グループ駅マップ
    
    usePresetCarouselData->>JotaiState: carouselData構築・更新
    usePresetCarouselData-->>SelectLineScreen: carouselData, routes, isRoutesDBInitialized
Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

Poem

🐰 ✨ 新しい5つのフック、データ流れを整理して
SelectLineScreenのからみつく枝を、すっきり分割
グラフQL、状態、位置情報、ウォークスルーまで
それぞれのフックが役割を果たし、画面はシンプルに
コードは呼吸するようにな流れになったね 🎉

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed プルリクエストのタイトルは日本語で「SelectLineScreenの分割リファクタリング」であり、提供されたサマリーに基づいて、SelectLineScreenコンポーネントを複数のカスタムフック(useInitialNearbyStation、useLineSelection、usePresetCarouselData、useStationsCache、useSelectLineWalkthrough)に分割するリファクタリングを行っていることが確認できます。このタイトルはこの変更の主要な目的を明確に表現しており、変更セットの中心的な内容と正確に一致しています。
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chore/select-line-screen-refactoring

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (11)
src/hooks/useSelectLineWalkthrough.ts (2)

64-73: マジックナンバーの抽出を検討してください。

-16 の補正値は意図がコメントで説明されていますが、定数として抽出するとより明確になります。

♻️ 提案
+const PRESETS_MARGIN_TOP_OFFSET = 16;
+
 // プリセットエリアをハイライト(marginTop: -16 を補正)
 useEffect(() => {
   if (currentStepId === 'savedRoutes' && presetsLayout) {
     setSpotlightArea({
       x: presetsLayout.x,
-      y: presetsLayout.y - 16,
+      y: presetsLayout.y - PRESETS_MARGIN_TOP_OFFSET,
       width: presetsLayout.width,
       height: presetsLayout.height,
       borderRadius: 12,
     });
   }
 }, [currentStepId, presetsLayout, setSpotlightArea]);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/hooks/useSelectLineWalkthrough.ts` around lines 64 - 73, Extract the
magic number -16 used when computing the spotlight Y position into a well-named
constant (e.g. SPOTLIGHT_VERTICAL_OFFSET or SPOTLIGHT_Y_ADJUSTMENT) and replace
the literal in useSelectLineWalkthrough's effect where currentStepId ===
'savedRoutes' sets the spotlight (referencing presetsLayout and
setSpotlightArea). Define the constant near the top of the hook (or with other
related constants) and include a short comment explaining why the offset is
needed so the intent is clear.

7-12: Layout 型の重複を検討してください。

ButtonLayout(FooterTabBar.tsx)や HeaderLayout(NowHeader.tsx)と同一の構造です。共通の型を再利用するか、共通の場所に定義することで重複を削減できます。

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/hooks/useSelectLineWalkthrough.ts` around lines 7 - 12, The Layout type
is duplicated across components; extract a single shared type (e.g., export type
Rect = { x: number; y: number; width: number; height: number } or export type
LayoutRect = ...) into a common types module and replace the local Layout,
ButtonLayout and HeaderLayout aliases with this shared type so all usages
(Layout, ButtonLayout, HeaderLayout) import the common type; update the
declarations in useSelectLineWalkthrough.ts, FooterTabBar.tsx, and NowHeader.tsx
to import and use the new shared type and remove the redundant local
definitions.
src/hooks/useInitialNearbyStation.test.tsx (1)

56-61: HookBridge パターンについて

HookBridge コンポーネントパターンはフックの同期的な結果取得に有効ですが、@testing-library/react-nativerenderHook を使用すると、より標準的なテストパターンになります(他のテストファイルで使用されているように)。

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/hooks/useInitialNearbyStation.test.tsx` around lines 56 - 61, Replace the
custom HookBridge component pattern with the standard renderHook usage from the
testing library: remove the HookBridge component and instead call renderHook(()
=> useInitialNearbyStation(), { wrapper: ... }) (or with appropriate provider
wrapper used in other tests), then access result.current (or await
waitForNextUpdate/waitFor) to assert hook output; refer to the HookBridge
component and the useInitialNearbyStation hook in the test to find where to
substitute renderHook so the test follows the same pattern as other files.
src/hooks/usePresetCarouselData.ts (1)

33-104: レースコンディションの可能性があります。

routes が急速に変更された場合、複数の fetchAsync が並行して実行される可能性があります。prevRoutesKeyRef.currentsetCarouselData の後に更新されるため、古いフェッチの結果が新しいデータを上書きする可能性があります。

♻️ 提案: フェッチ開始時に routesKey を保存
 useEffect(() => {
   const routesKey = routes
     .map((r) => `${r.id}:${r.lineId}:${r.trainTypeId}:${r.hasTrainType}`)
     .join(',');
   if (routesKey === prevRoutesKeyRef.current) return;
+  prevRoutesKeyRef.current = routesKey;

   const fetchAsync = async () => {
     try {
       // ... fetch logic ...

       setCarouselData(
         routes.map((r, i) => ({
           ...r,
           __k: `${r.id}-${i}`,
           stations: r.hasTrainType
             ? (trainTypeStationsMap.get(r.trainTypeId) ?? [])
             : (lineStationsMap.get(r.lineId) ?? []),
         }))
       );
-      prevRoutesKeyRef.current = routesKey;
     } catch (err) {
       console.error(err);
     }
   };
   fetchAsync();
 }, [routes]);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/hooks/usePresetCarouselData.ts` around lines 33 - 104, The effect can
suffer a race where an earlier fetchAsync overwrites newer results; fix by
capturing this run's routesKey into a local constant (e.g., const myKey =
routesKey) and use a ref to mark the latest fetch (e.g.,
latestFetchKeyRef.current = myKey) before awaiting; then, right before calling
setCarouselData and updating prevRoutesKeyRef.current, confirm
latestFetchKeyRef.current === myKey so only the most recent fetch writes state,
and update prevRoutesKeyRef.current = myKey when you setCarouselData; reference
useEffect, fetchAsync, prevRoutesKeyRef, setCarouselData and add
latestFetchKeyRef to implement this guard.
src/screens/SelectLineScreen.render.test.tsx (2)

269-272: afterEach での jest.clearAllMocks() 呼び出しを検討してください

コーディングガイドラインでは afterEach フックで jest.clearAllMocks() を呼び出すことが推奨されています。beforeEach で呼び出すと、テスト失敗時のデバッグ時にモックの状態が既にクリアされている場合があります。

♻️ 修正案
 describe('SelectLineScreen', () => {
-  beforeEach(() => {
+  afterEach(() => {
     jest.clearAllMocks();
   });

As per coding guidelines: "Call jest.clearAllMocks() in afterEach hook"

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/screens/SelectLineScreen.render.test.tsx` around lines 269 - 272, In the
describe('SelectLineScreen') test suite, move the jest.clearAllMocks() call out
of the beforeEach hook and into an afterEach hook so mocks are cleared after
each test rather than before; update the suite to remove jest.clearAllMocks()
from beforeEach and add afterEach(() => jest.clearAllMocks()) to ensure mock
state is preserved during test failure debugging.

258-264: atom の呼び出し順序に依存するモック実装

atomCallCount を使った実装は、フック内での useAtomValue の呼び出し順序に暗黙的に依存しています。SelectLineScreen の実装変更で順序が変わると、テストが失敗する可能性があります。

♻️ 修正案: atom を個別にモックする
-  // stationState を1回目、isLEDThemeAtom を2回目に返す
-  let atomCallCount = 0;
-  (useAtomValue as jest.Mock).mockImplementation(() => {
-    atomCallCount++;
-    if (atomCallCount % 2 === 1) return { stationsCache };
-    return false; // isLEDTheme
-  });
+  (useAtomValue as jest.Mock).mockImplementation((atom) => {
+    // atom の参照で判定する方がより堅牢
+    // 実際の atom をインポートして比較するか、
+    // 呼び出し引数のパターンで判定
+    return { stationsCache }; // 簡易版
+  });
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/screens/SelectLineScreen.render.test.tsx` around lines 258 - 264, The
test currently mocks useAtomValue with a call-count based implementation
(atomCallCount) which makes the test fragile to call-order changes; instead
update the mock of useAtomValue to inspect its atom argument and return per-atom
values (e.g. return { stationsCache } when the atom equals stationState or the
specific station atom, and return false when the atom equals isLEDThemeAtom), so
locate the mock of useAtomValue in the test and replace the call-count logic
with an argument-based branching on the atom parameter (referencing
useAtomValue, isLEDThemeAtom and the station/state symbol that provides
stationsCache).
src/hooks/useLineSelection.ts (3)

108-111: 不要な null 合体演算子

line はパラメータとして渡されているため、常に定義されています。line ?? null は冗長です。

♻️ 修正案
       setLineState((prev) => ({
         ...prev,
-        pendingLine: line ?? null,
+        pendingLine: line,
       }));
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/hooks/useLineSelection.ts` around lines 108 - 111, 不要な null
合体演算子が使われています: setLineState の更新で受け取ったパラメータ line は必ず定義されるため `pendingLine: line ??
null` は冗長です。useLineSelection.ts 内の該当箇所(setLineState の更新)を修正して `pendingLine:
line` に置き換え、余分な null 合体演算子を削除してください(関数名: setLineState、プロパティ: pendingLine)。

291-295: station.id の型安全性

station.id as number でキャストしていますが、station.idnull の可能性がある場合、ランタイムエラーの原因になります。

♻️ null チェックの追加
+      if (station.id == null) return;
+
       const fetchedTrainTypesData = await fetchTrainTypes({
         variables: {
-          stationId: station.id as number,
+          stationId: station.id,
         },
       });
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/hooks/useLineSelection.ts` around lines 291 - 295, The code casts
station.id with "as number" before calling fetchTrainTypes which can cause
runtime errors if station.id is null; in useLineSelection.ts guard against
null/undefined by checking station?.id (or station.id != null) before calling
fetchTrainTypes (e.g., early return, throw, or skip fetch), and pass the
validated number to fetchTrainTypes.variables.stationId so you never rely on a
forced cast; update the fetch call site and any control flow in the function to
handle the case where station.id is missing.

247-259: geolib の座標フォーマットの統一を検討してください

orderByDistance{ lat, lon } 形式を使用していますが、同じファイル内の findNearest (192-197 行) では { latitude, longitude } 形式を使用しています。geolib は両方のフォーマットを受け入れるため機能上の問題はありませんが、コード内で座標フォーマットが一貫していないと可読性に影響します。

♻️ 座標フォーマットの統一
       const sortedStationCoords =
         latitude && longitude
           ? (orderByDistance(
-              { lat: latitude, lon: longitude },
+              { latitude, longitude },
               stations.map((sta) => ({
                 latitude: sta.latitude as number,
                 longitude: sta.longitude as number,
               }))
             ) as { latitude: number; longitude: number }[])
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/hooks/useLineSelection.ts` around lines 247 - 259, The code uses two
different geolib coordinate shapes; make them consistent by changing the
orderByDistance call in sortedStationCoords to accept the same { latitude,
longitude } format used by findNearest: update the reference point and the
stations.map transformation inside the orderByDistance invocation to produce {
latitude, longitude } keys (and adjust the expected cast/type if needed) so all
coordinate handling in useLineSelection.ts uses the same shape (refer to
sortedStationCoords and findNearest to align formats).
src/hooks/useLineSelection.test.tsx (1)

74-287: handlePresetPress のテストケースが不足しています

handleLineSelectedhandleTrainTypeSelecthandleCloseSelectBoundModal はテストされていますが、handlePresetPress のテストがありません。

handlePresetPress のテストケースを追加しますか?この関数は route.hasTrainType に基づいて openModalByTrainTypeId または openModalByLineId を呼び出す分岐があります。

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/hooks/useLineSelection.test.tsx` around lines 74 - 287, Add tests for
handlePresetPress to cover both branches: when route.hasTrainType is true it
should call openModalByTrainTypeId, and when false it should call
openModalByLineId. In each test use setupMolecules()/setupQueries(), render
HookBridge to get hookRef, pass a route object with hasTrainType true/false and
appropriate trainTypeId/lineId, mock or spy the openModalByTrainTypeId and
openModalByLineId functions (and ensure other modal function is not called),
invoke hookRef.current?.handlePresetPress(route) inside act, and assert the
correct modal function was called with the expected id while the other was not.
src/hooks/useInitialNearbyStation.ts (1)

46-56: バックグラウンド位置更新の停止処理

stopLocationUpdates の非同期処理でエラーが発生した場合のハンドリングがありません。

♻️ エラーハンドリングの追加
   useEffect(() => {
     const stopLocationUpdates = async () => {
-      const hasStartedLocationUpdates =
-        await Location.hasStartedLocationUpdatesAsync(LOCATION_TASK_NAME);
-      if (hasStartedLocationUpdates) {
-        await Location.stopLocationUpdatesAsync(LOCATION_TASK_NAME);
-      }
+      try {
+        const hasStartedLocationUpdates =
+          await Location.hasStartedLocationUpdatesAsync(LOCATION_TASK_NAME);
+        if (hasStartedLocationUpdates) {
+          await Location.stopLocationUpdatesAsync(LOCATION_TASK_NAME);
+        }
+      } catch (error) {
+        console.error('Failed to stop location updates:', error);
+      }
     };
     stopLocationUpdates();
   }, []);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/hooks/useInitialNearbyStation.ts` around lines 46 - 56, The async
stopLocationUpdates function inside the useEffect lacks error handling; wrap the
body of stopLocationUpdates in a try/catch so failures from
Location.hasStartedLocationUpdatesAsync or Location.stopLocationUpdatesAsync are
caught and handled (e.g., log the error or surface it to a state/error
reporter). Keep the existing flow using LOCATION_TASK_NAME and ensure the catch
logs which call failed and the error object so debugging is possible while
preserving the current behavior when no updates are started.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/hooks/useInitialNearbyStation.test.tsx`:
- Around line 56-61: Replace the custom HookBridge component pattern with the
standard renderHook usage from the testing library: remove the HookBridge
component and instead call renderHook(() => useInitialNearbyStation(), {
wrapper: ... }) (or with appropriate provider wrapper used in other tests), then
access result.current (or await waitForNextUpdate/waitFor) to assert hook
output; refer to the HookBridge component and the useInitialNearbyStation hook
in the test to find where to substitute renderHook so the test follows the same
pattern as other files.

In `@src/hooks/useInitialNearbyStation.ts`:
- Around line 46-56: The async stopLocationUpdates function inside the useEffect
lacks error handling; wrap the body of stopLocationUpdates in a try/catch so
failures from Location.hasStartedLocationUpdatesAsync or
Location.stopLocationUpdatesAsync are caught and handled (e.g., log the error or
surface it to a state/error reporter). Keep the existing flow using
LOCATION_TASK_NAME and ensure the catch logs which call failed and the error
object so debugging is possible while preserving the current behavior when no
updates are started.

In `@src/hooks/useLineSelection.test.tsx`:
- Around line 74-287: Add tests for handlePresetPress to cover both branches:
when route.hasTrainType is true it should call openModalByTrainTypeId, and when
false it should call openModalByLineId. In each test use
setupMolecules()/setupQueries(), render HookBridge to get hookRef, pass a route
object with hasTrainType true/false and appropriate trainTypeId/lineId, mock or
spy the openModalByTrainTypeId and openModalByLineId functions (and ensure other
modal function is not called), invoke hookRef.current?.handlePresetPress(route)
inside act, and assert the correct modal function was called with the expected
id while the other was not.

In `@src/hooks/useLineSelection.ts`:
- Around line 108-111: 不要な null 合体演算子が使われています: setLineState の更新で受け取ったパラメータ line
は必ず定義されるため `pendingLine: line ?? null` は冗長です。useLineSelection.ts
内の該当箇所(setLineState の更新)を修正して `pendingLine: line` に置き換え、余分な null
合体演算子を削除してください(関数名: setLineState、プロパティ: pendingLine)。
- Around line 291-295: The code casts station.id with "as number" before calling
fetchTrainTypes which can cause runtime errors if station.id is null; in
useLineSelection.ts guard against null/undefined by checking station?.id (or
station.id != null) before calling fetchTrainTypes (e.g., early return, throw,
or skip fetch), and pass the validated number to
fetchTrainTypes.variables.stationId so you never rely on a forced cast; update
the fetch call site and any control flow in the function to handle the case
where station.id is missing.
- Around line 247-259: The code uses two different geolib coordinate shapes;
make them consistent by changing the orderByDistance call in sortedStationCoords
to accept the same { latitude, longitude } format used by findNearest: update
the reference point and the stations.map transformation inside the
orderByDistance invocation to produce { latitude, longitude } keys (and adjust
the expected cast/type if needed) so all coordinate handling in
useLineSelection.ts uses the same shape (refer to sortedStationCoords and
findNearest to align formats).

In `@src/hooks/usePresetCarouselData.ts`:
- Around line 33-104: The effect can suffer a race where an earlier fetchAsync
overwrites newer results; fix by capturing this run's routesKey into a local
constant (e.g., const myKey = routesKey) and use a ref to mark the latest fetch
(e.g., latestFetchKeyRef.current = myKey) before awaiting; then, right before
calling setCarouselData and updating prevRoutesKeyRef.current, confirm
latestFetchKeyRef.current === myKey so only the most recent fetch writes state,
and update prevRoutesKeyRef.current = myKey when you setCarouselData; reference
useEffect, fetchAsync, prevRoutesKeyRef, setCarouselData and add
latestFetchKeyRef to implement this guard.

In `@src/hooks/useSelectLineWalkthrough.ts`:
- Around line 64-73: Extract the magic number -16 used when computing the
spotlight Y position into a well-named constant (e.g. SPOTLIGHT_VERTICAL_OFFSET
or SPOTLIGHT_Y_ADJUSTMENT) and replace the literal in useSelectLineWalkthrough's
effect where currentStepId === 'savedRoutes' sets the spotlight (referencing
presetsLayout and setSpotlightArea). Define the constant near the top of the
hook (or with other related constants) and include a short comment explaining
why the offset is needed so the intent is clear.
- Around line 7-12: The Layout type is duplicated across components; extract a
single shared type (e.g., export type Rect = { x: number; y: number; width:
number; height: number } or export type LayoutRect = ...) into a common types
module and replace the local Layout, ButtonLayout and HeaderLayout aliases with
this shared type so all usages (Layout, ButtonLayout, HeaderLayout) import the
common type; update the declarations in useSelectLineWalkthrough.ts,
FooterTabBar.tsx, and NowHeader.tsx to import and use the new shared type and
remove the redundant local definitions.

In `@src/screens/SelectLineScreen.render.test.tsx`:
- Around line 269-272: In the describe('SelectLineScreen') test suite, move the
jest.clearAllMocks() call out of the beforeEach hook and into an afterEach hook
so mocks are cleared after each test rather than before; update the suite to
remove jest.clearAllMocks() from beforeEach and add afterEach(() =>
jest.clearAllMocks()) to ensure mock state is preserved during test failure
debugging.
- Around line 258-264: The test currently mocks useAtomValue with a call-count
based implementation (atomCallCount) which makes the test fragile to call-order
changes; instead update the mock of useAtomValue to inspect its atom argument
and return per-atom values (e.g. return { stationsCache } when the atom equals
stationState or the specific station atom, and return false when the atom equals
isLEDThemeAtom), so locate the mock of useAtomValue in the test and replace the
call-count logic with an argument-based branching on the atom parameter
(referencing useAtomValue, isLEDThemeAtom and the station/state symbol that
provides stationsCache).

@TinyKitten
Copy link
Member Author

@coderabbitai nitpickコメントをまとめて起票してください

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 20, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch all review comments from PR `#5368`
gh pr view 5368 --json reviews,comments --jq '.reviews[] | select(.author.login == "coderabbitai[bot]") | .body'

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Also fetch review comments (inline comments on code)
gh api repos/TrainLCD/MobileApp/pulls/5368/comments --jq '.[] | select(.user.login == "coderabbitai[bot]") | {path: .path, line: .line, body: .body}'

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Fetch all data about PR `#5368` to understand what comments exist
gh pr view 5368 --json number,title,state,reviews,comments

Length of output: 46001


🏁 Script executed:

#!/bin/bash
# Check for any review threads/comments
gh api repos/TrainLCD/MobileApp/pulls/5368/reviews

Length of output: 24884


@TinyKitten: nitpickコメントをまとめてissueを起票します。


---

🏁 Script executed:

#!/bin/bash

Create GitHub issue with consolidated nitpick comments

gh issue create
--title "PR #5368 nitpickコメント対応"
--body "## 概要
PR #5368 (SelectLineScreenの分割リファクタリング) のレビューで指摘されたnitpickコメントをまとめたissueです。

関連PR: #5368
リクエスト元: @TinyKitten


対応項目

src/hooks/useSelectLineWalkthrough.ts (2件)

  1. マジックナンバーの抽出 (64-73行目)

    • `-16` の補正値を定数として抽出する(例: `PRESETS_MARGIN_TOP_OFFSET`)
    • 意図を明確にするためのコメントを追加
  2. Layout型の重複 (7-12行目)

    • `Layout`、`ButtonLayout`、`HeaderLayout` が重複している
    • 共通の型定義を作成して再利用する

src/hooks/useInitialNearbyStation.test.tsx (1件)

  1. HookBridgeパターン (56-61行目)
    • `@testing-library/react-native` の `renderHook` を使用した標準的なテストパターンに変更

src/hooks/usePresetCarouselData.ts (1件)

  1. レースコンディションの可能性 (33-104行目)
    • `routes` が急速に変更された場合の競合状態を防ぐ
    • フェッチ開始時に `routesKey` を保存し、最新のフェッチ結果のみを反映

src/screens/SelectLineScreen.render.test.tsx (2件)

  1. jest.clearAllMocks()の呼び出し位置 (269-272行目)

    • `beforeEach` から `afterEach` に移動
    • テスト失敗時のデバッグでモック状態を保持
  2. atomの呼び出し順序に依存するモック (258-264行目)

    • `atomCallCount` ベースの実装を、atom引数を判定する実装に変更
    • 呼び出し順序の変更に対して堅牢にする

src/hooks/useLineSelection.ts (3件)

  1. 不要なnull合体演算子 (108-111行目)

    • `pendingLine: line ?? null` を `pendingLine: line` に簡素化
  2. station.idの型安全性 (291-295行目)

    • `station.id as number` のキャストの前に null チェックを追加
    • ランタイムエラーを防止
  3. geolibの座標フォーマット統一 (247-259行目)

    • `{ lat, lon }` と `{ latitude, longitude }` が混在
    • `{ latitude, longitude }` 形式に統一

src/hooks/useLineSelection.test.tsx (1件)

  1. handlePresetPressのテストケース不足 (74-287行目)
    • `route.hasTrainType` が true/false の両方のケースをカバー
    • `openModalByTrainTypeId` と `openModalByLineId` の呼び出しをテスト

src/hooks/useInitialNearbyStation.ts (1件)

  1. バックグラウンド位置更新のエラーハンドリング (46-56行目)
    • `stopLocationUpdates` の非同期処理に try/catch を追加
    • エラー発生時のログ出力を実装

優先度

このissueはコードの品質向上を目的としており、緊急性は低いです。時間のある時に対応してください。"
--assignee TinyKitten
--repo TrainLCD/MobileApp


Length of output: 49

---



</details>

`@TinyKitten`: nitpickコメントをまとめたissueを作成しました。

**Issue `#5372`**: https://github.com/TrainLCD/MobileApp/issues/5372

以下の11件のnitpickコメントを含んでいます:
- `useSelectLineWalkthrough.ts`: マジックナンバーの抽出、Layout型の重複 (2件)
- `useInitialNearbyStation.test.tsx`: HookBridgeパターン (1件)
- `usePresetCarouselData.ts`: レースコンディション (1件)
- `SelectLineScreen.render.test.tsx`: テストのベストプラクティス (2件)
- `useLineSelection.ts`: 型安全性とコードの簡素化 (3件)
- `useLineSelection.test.tsx`: テストカバレッジ (1件)
- `useInitialNearbyStation.ts`: エラーハンドリング (1件)

優先度は低めですので、お時間のある時に対応いただければと思います。

---

<details>
<summary>🧠 Learnings used</summary>

Learnt from: TinyKitten
Repo: TrainLCD/MobileApp PR: 5102
File: .env:0-0
Timestamp: 2026-01-18T01:25:10.303Z
Learning: TrainLCD/MobileAppのレビューコメントでは、CodeRabbitの組み込みツール(dotenv-linter等)による指摘と、プロジェクト固有のコーディング規約による指摘を明確に区別して記載すること。組み込みツールの指摘であることを明示的に伝えること。


---

Learnt from: TinyKitten
Repo: TrainLCD/MobileApp PR: 0
File: :0-0
Timestamp: 2025-08-11T21:13:39.083Z
Learning: TinyKitten prefers concise code reviews for the TrainLCD/MobileApp repository. Keep review comments brief and to the point.


---

Learnt from: CR
Repo: TrainLCD/MobileApp PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-25T03:10:26.328Z
Learning: Code review and feedback should be provided in Japanese


---

Learnt from: CR
Repo: TrainLCD/MobileApp PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-25T03:10:26.328Z
Learning: Write comments in Japanese or English, but maintain consistency throughout the codebase


---

Learnt from: CR
Repo: TrainLCD/MobileApp PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-25T03:10:26.328Z
Learning: Comment language can be Japanese or English, but consistency must be maintained


</details>

@TinyKitten TinyKitten merged commit 20757f3 into dev Feb 20, 2026
7 checks passed
@TinyKitten TinyKitten deleted the chore/select-line-screen-refactoring branch February 20, 2026 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant